Restore RT2 RenderCfg fields removed in PhysicsManager refactor#5167
Restore RT2 RenderCfg fields removed in PhysicsManager refactor#5167nblauch wants to merge 5 commits intoisaac-sim:developfrom
Conversation
PR isaac-sim#4142 added 10 RT2 typed fields to RenderCfg (max_bounces, split_glass, split_clearcoat, split_rough_reflection, ambient_light_intensity, ambient_occlusion_denoiser_mode, subpixel_mode, enable_cached_raytracing, max_samples_per_launch, view_tile_limit) and their field_to_setting mappings. These were accidentally removed in the PhysicsManager refactor (PR isaac-sim#4564, commit 0ba9c5c). This restores them along with test coverage.
nblauch
requested review from
kellyguo11,
matthewtrepte and
ossamaAhmed
as code owners
Greptile SummaryThis PR restores 10 RT2 ( Two minor issues to be aware of: the docstrings for Confidence Score: 5/5Safe to merge; the core field restoration and carb-path wiring are correct, and all remaining findings are P2. All findings are P2: two docstring/test default inconsistencies and skipped tests. The restored cfg fields and field_to_setting mappings are correct, matching the original PR #4142 intent and the kit preset files. test_simulation_render_config.py — the RT2-specific tests are skipped and the default values used in test_render_cfg_defaults contradict two docstrings. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[RenderCfg field set\ne.g. max_bounces=4] --> B{value is None?}
B -- Yes --> C[Skip field]
B -- No --> D{in excluded keys?\nrendering_mode / carb_settings / antialiasing_mode}
D -- Yes --> E[Handled separately]
D -- No --> F[field_to_setting lookup]
F --> G{mapping found?}
G -- No --> H[Unknown field, skipped]
G -- Yes --> I[set_setting carb path\ne.g. /rtx/rtpt/maxBounces]
I --> J[SettingsHelper.set\nauto-types bool/int/float/str]
J --> K[SettingsManager writes\nto carb runtime]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| split_rough_reflection: bool | None = None | ||
| """Enables separate rough reflection ray splitting (RT2). Default is True. | ||
|
|
||
| Enabling this can reduce noise on rough reflective materials at the cost of performance. | ||
|
|
||
| This is set by the variable: ``/rtx/rtpt/splitRoughReflection``. | ||
| """ |
There was a problem hiding this comment.
split_rough_reflection is documented as "Default is True", but test_render_cfg_defaults (line 213) uses False as the expected default value under the # RT2 defaults comment. Similarly, enable_cached_raytracing is documented as "Default is True" (line 215) while the test uses False (line 216). One of these is wrong — either the carb-setting system default changed, or the docstrings are incorrect. If the actual carb defaults are False, the docstrings should read "Default is False".
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good restoration PR — the fields, carb path mappings, and test coverage all look correct. One docstring was changed from the original PR #4142 (not a faithful restoration). CI still running (pre-commit, license-check in progress).
| Enabling this can reduce noise on rough reflective materials at the cost of performance. | ||
|
|
||
| This is set by the variable: ``/rtx/rtpt/splitRoughReflection``. | ||
| """ |
There was a problem hiding this comment.
The original PR #4142 had this as "Default is False", and test_render_cfg_defaults also uses False. All three .kit presets (performance, balanced, quality) explicitly set splitRoughReflection = true, which strongly implies the carb-level default is false (otherwise the presets wouldn't need to set it).
This should be restored to match the original:
| """ | |
| """Enables separate rough reflection ray splitting (RT2). Default is False. |
|
look like kit startup performance has been broken maybe we need to relax the timing? |
|
the start up timing seems to be failing is all prs, so not this pr issue |
|
hi @kellyguo11 @ooctipus @AntoineRichard wondering what the status on this is? seems important |
|
i wonder if we should also remove the unused legacy RT1 settings |
I think we should remove them, we don't want users to use RT1 anymore. do you mind updating that in the PR as well? the PR is approved already so you should be able to merge when things are ready. |
4 participants
Description
PR #4142 added 10 typed RT2 (
RealTimePathTracing) fields toRenderCfgto expose the carb settings that actually control rendering quality under the RT2 mode that has been default since Isaac Sim 4.5. The PhysicsManager refactor in PR #4564 (commit0ba9c5cb) accidentally removed all 10 of these fields during a largesimulation_cfg.pyrewrite, while the.kitpreset files that reference the same carb paths were left intact.This PR restores the removed fields and their
field_to_settingmappings, along with test coverage.Restored fields:
max_bounces/rtx/rtpt/maxBouncessplit_glass/rtx/rtpt/splitGlasssplit_clearcoat/rtx/rtpt/splitClearcoatsplit_rough_reflection/rtx/rtpt/splitRoughReflectionambient_light_intensity/rtx/sceneDb/ambientLightIntensityambient_occlusion_denoiser_mode/rtx/ambientOcclusion/denoiserModesubpixel_mode/rtx/raytracing/subpixel/modeenable_cached_raytracing/rtx/raytracing/cached/enabledmax_samples_per_launch/rtx/pathtracing/maxSamplesPerLaunchview_tile_limit/rtx/viewTile/limitWithout these fields, users can only reach RT2 quality knobs through the
carb_settingsescape-hatch dict, while the existing named fields (samples_per_pixel,enable_translucency,enable_reflections,enable_direct_lighting) map to RT1 Legacy carb paths that RT2 ignores entirely.Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there